feat(ICRC-Ledger): DEFI-1687: implement ICRC-107 fee collector#8217
feat(ICRC-Ledger): DEFI-1687: implement ICRC-107 fee collector#8217maciejdfinity wants to merge 129 commits intomasterfrom
Conversation
maciejdfinity
changed the title
mbjorkqvist
left a comment
mbjorkqvist
left a comment
There was a problem hiding this comment.
Thanks @maciejdfinity! I have a few comments and questions, but overall this looks very good!
| "ck_btc_ledger": "ic-icrc1-ledger.wasm.gz", | ||
| "ck_btc_ledger_v1": "ic-icrc1-ledger.wasm.gz", | ||
| "ck_btc_ledger_v3": "ic-icrc1-ledger.wasm.gz", | ||
| "ck_btc_ledger_v5": "ic-icrc1-ledger.wasm.gz", |
There was a problem hiding this comment.
Would it make sense to rename this to something more descriptive, and/or perhaps add a comment somewhere (I think comments don't work in mainnet-canister-revisions.json, but here would be ok?)? Especially since the v5 doesn't correspond to the LEDGER_VERSION, so it can be confusing.
There was a problem hiding this comment.
Good point! Maybe we could change it to correspond to the ledger versions, i.e.:
v1 -> no_mem_mgr
v3 -> v1
v5 -> v3
WDYT?
| "ck_btc_ledger_v5": { | ||
| "rev": "512cf412f33d430b79f42330518166d14fc6884e", | ||
| "sha256": "901bc548f901145bd15a1156487eed703705794ad6a23787eaa04b1c7bbdcf48" | ||
| }, |
There was a problem hiding this comment.
This points to ledger-suite-icrc-2025-04-14. Is there a particular reason for this specific version, or could we use ledger-suite-icrc-2025-10-27, which is the minimum version we point to from the currently most recent version ledger-suite-icrc-2026-02-02?
There was a problem hiding this comment.
No reason, I just used the same version as current mainnet ledger :) Changed!
There was a problem hiding this comment.
Would it make sense to move all the fee collector-related tests to a separate file? rs/ledger_suite/icrc1/index-ng/tests/tests.rs is already over 2000 lines.
Also, would it make sense to add tests for the following (unless they already exist and I missed them):
- Trying to set the fee collector account to the minting account, the anonymous account, and an invalid account (e.g., an account with an invalid principal). For the init, upgrade, and endpoint fee collector setting paths. IIUC, there's a check for the minting account in the init and upgrade paths, but not for the endpoint?
There was a problem hiding this comment.
Good catch, I did not check for minting or anonymous account in the endpoint and init/upgrade did not check for the anonymous account. I added test_fee_collector_107_minting_account and test_fee_collector_107_anonymous which cover the 6 cases: (minter, anonymous)x(endpoint, init, upgrade).
As for the invalid Account - since this is a candid endpoint, I don't think we have to check that - we always receive a valid Account, invalid Accounts should be rejected by cdk?
I also moved the index tests to a separate file.
| // Downgrade to mainnet is currently not possible. The rest of the test can | ||
| // be uncommented again, once the PR that introduced this line is on mainnet. | ||
|
|
||
| // // Downgrade the ledger to the mainnet version that does not support ICRC-106 | ||
| // env.upgrade_canister(canister_id, mainnet_ledger_wasm, encoded_empty_upgrade_args) | ||
| // .expect("should successfully downgrade ledger canister"); | ||
| // assert_index_not_set(&env, canister_id, false); | ||
|
|
||
| // // Upgrade to a ledger version that supports ICRC-106, but do not set the index principal | ||
| // let encoded_empty_upgrade_args = Encode!(&encode_empty_upgrade_args()).unwrap(); | ||
| // env.upgrade_canister(canister_id, ledger_wasm, encoded_empty_upgrade_args) | ||
| // .expect("should successfully upgrade ledger canister"); | ||
| // assert_index_not_set(&env, canister_id, true); |
There was a problem hiding this comment.
Would it help to have a ticket in the ICRC-107 epic to remember to uncomment this, and the other snippet in rs/ledger_suite/icrc1/tests/upgrade_downgrade.rs, once this has been released and deployed to mainnet? Or will the tests fail anyway once that happens, and uncommenting cannot be forgotten?
There was a problem hiding this comment.
It could be forgotten, I created https://dfinity.atlassian.net/browse/DEFI-2657
Co-authored-by: Mathias Björkqvist <mathias.bjorkqvist@dfinity.org>
2 participants
With this change the ICRC ledger now implements the ICRC-107 fee collector standard as described in https://github.com/dfinity/ICRC/blob/main/ICRCs/ICRC-107/ICRC-107.md. The legacy fee collector is no longer supported by the ledger. This results in the following notable changes:
The fee collector can be configured using the existing
fee_collector_accountinit argument andchange_fee_collectorupgrade argument.Since the ICRC-107 fee collector change is not backwards compatible, the ledger upgraded to this version cannot be downgraded to an earlier version.